Use includes instead of system_includes for includes attr
#4
Conversation
Previously even though the attribute was named `includes` it was passed through the `system_includes` field of the compilation context. This resulted in toolchains passing these include paths with `-isystem`, which is unexpected if you use this for first party code. Many non bazel-first projects have header directory structures that require custom include paths be propagated throughout the graph, the alternative to `includes` is to use `strip_include_prefix`. The downside of `strip_include_prefix` is that you add 1 include path per `cc_library`, even if the libraries are in the same package. With `includes` these are deduplicated. In the case of LLVM using `includes` reduced the number of search paths on the order of hundreds. If users want to use `-isystem` for third party code that uses `includes`, they can pass `--features=external_include_paths --host_features=external_include_paths` If there are first party libraries users want to use `-isystem` with, they can use `features = ["system_include_paths"]` Fixes bazelbuild#20267 RELNOTES[INC]: Use `-I` instead of `-isystem` for `cc_library` / `cc_binary` `includes` attr. To use `-isystem` for only external repositories, you can pass `--features=external_include_paths --host_features=external_include_paths`. To use `-isystem` for a single `cc_library` / `cc_binary` `includes`, you can set `features = ["system_include_paths"],` on the target
|
this requires a toolchain change in order to add a test, I can do that if folks are happy with this direction |
|
see the alternative here bazelbuild#25751 |
|
Selfishly, we have on the order of a dozen+ cc_toolchains that will need to add the I'm not opposed in principle, but can we add an incompatible flag for this? Or keep the current behavior if a feature is unset? Open to suggestions, but I'd like to have a way to disable this globally (for our monorepo) without having to modify many toolchains. |
|
totally fair, I can add this behind an incompatible flag, before I do that on this one, should we consider bazelbuild#25751 or should I close that and move forward with this approach? |
|
it looks like there is another implementation option, we can use |
|
I've updated to the |
|
ok the CI failures here lead to some interesting discoveries. for one the BUILD files of both grpc and c-ares have undeclared inclusion errors lurking that only hit if you use the change here is that previously those were ignored because The fix here is to pass I've added a release notes blurb as well |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis set of changes updates the handling of include directories in Bazel's C++ and Objective-C build rules. The code and build scripts are modified to shift from using "system include" directories to more general "include" directories in both the build logic and associated tests. Several scripts and Starlark files now use Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bazel
participant StarlarkRule
participant CppCompileAction
User->>Bazel: Starts build with updated flags
Bazel->>StarlarkRule: Invokes cc_binary/cc_library/objc rules
StarlarkRule->>StarlarkRule: Calls cc_helper.include_dirs(...)
StarlarkRule->>CppCompileAction: Passes include dirs as 'includes'
CppCompileAction->>CppCompileAction: Ignores built-in, system, and external include dirs during validation
CppCompileAction-->>Bazel: Compilation proceeds with updated include handling
Assessment against linked issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.31.1)src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
🔇 Additional comments (16)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Previously even though the attribute was named
includesit was passedthrough the
system_includesfield of the compilation context. Thisresulted in toolchains passing these include paths with
-isystem,which is unexpected if you use this for first party code.
Many non bazel-first projects have header directory structures that
require custom include paths be propagated throughout the graph, the
alternative to
includesis to usestrip_include_prefix. The downsideof
strip_include_prefixis that you add 1 include path percc_library, even if the libraries are in the same package. Withincludesthese are deduplicated. In the case of LLVM usingincludesreduced the number of search paths on the order of hundreds.
If users want to use
-isystemfor third party code that usesincludes, they can pass--features=external_include_paths --host_features=external_include_pathsIf there are first party libraries users want to use
-isystemwith,they can use
features = ["system_include_paths"]Fixes bazelbuild#20267
RELNOTES[INC]: Use
-Iinstead of-isystemforcc_library/cc_binaryincludesattr. To use-isystemfor only external repositories, you can pass--features=external_include_paths --host_features=external_include_paths. To use-isystemfor a singlecc_library/cc_binaryincludes, you can setfeatures = ["system_include_paths"],on the targetSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores